Default to first non-disabled option for select elements #10456
Conversation
Instead of defaulting to the first option for a select element, which could be a disabled option, we find the first non-disabled option available while we looping through the options. If no option matches, set the first non-disabled option as selected.
| if (options.length) { | ||
| options[0].selected = true; | ||
| if (defaultSelected !== null) { | ||
| defaultSelected.selected = true; |
There was a problem hiding this comment.
I know you didn't write all this, but the logic is hard to follow here...can we switch early to see if it's controlled?
There was a problem hiding this comment.
I agree it's a little dense, can you clarify what you mean by switching early to see if it's controlled? As far as I can tell, updateOptions only branches on multiple and doesn't really care if it's controlled or not.
There was a problem hiding this comment.
I was thinking check if propValue was set, but that wouldn't actually work...I guess i don't have any specific suggestions :P but I felt like I had to read this through 3 times to understand it, and it feels like it should be easier to express
|
What happens to browser features like attempts to automatically restoring state and auto-fill? Does it care? |
|
@aweary 🍒 ⛏ away. |
|
@sebmarkbage We don't have fixture coverage for restored state and auto-fill. This would be a good addition to the suite. I'll write up an issue. |
| @@ -101,14 +101,18 @@ function updateOptions( | |||
| // Do not set `select.value` as exact behavior isn't consistent across all | |||
| // browsers for all cases. | |||
There was a problem hiding this comment.
I wish I knew what wasn't consistent -- and where? Like was it IE8?. I'll write up an issue to do some archaeology here.
Totally unrelated to this PR 😛.
aweary
force-pushed
the
fix-select-default-value-regression
branch
from
f30b9b8
to
f2dea9d
Compare
|
What is the status on this? |
|
@gaearon I cherry-picked the test fixtures, so it should be good. Feel free to merge if it looks good to you. |
|
The fixture is broken for me. But I also get a runtime error:
Inside |
| <span className="hint" /> | ||
| </fieldset> | ||
| <fieldset> | ||
| <legend>Controlled in nested subtree</legend> |
There was a problem hiding this comment.
Was this case removed intentionally?
|
Thanks! |
|
Thanks for the fix @gaearon! |
) * Default to first non-disabled option for select Instead of defaulting to the first option for a select element, which could be a disabled option, we find the first non-disabled option available while we looping through the options. If no option matches, set the first non-disabled option as selected. * Add ReactDOMSelect test for defaulting to non-disabled options * Add test fixtures to cover disabled selected options * Fix bad merge
This fixes a regression that @nhunzaker found in #10142 where
selectelements were defaulting to the firstoption, regardless of whether it was disabled or not. This was not the behavior in 15.6. Copying my comment on this from Nate's PR:This brings us back in sync with the HTML spec which states:
https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element
@nhunzaker I can just cherry-pick the DOM fixtures from #10142 as part of this PR if you'd like!